Skip to content

fix: offer a parallel covering-projection scan - #1127

Merged
jdatcmd merged 5 commits into
commandprompt:mainfrom
linuxhikerpm:audit/covering-projection-parallel-path
Sep 22, 2026
Merged

jdatcmd merged 5 commits into
commandprompt:mainfrom
linuxhikerpm:audit/covering-projection-parallel-path

Conversation

@linuxhikerpm

Copy link
Copy Markdown

Summary

  • A covering projection path was a serial CustomPath (parallel_aware = false, parallel_safe = false) while the parallel base scan was a partial path with no projection name. Those cannot both be true of one plan: either Gather wins and the projection is dropped, or the serial projection wins and the workers are dropped.
  • Measured on this tree, PG18, scrambled 32,000-row table: under parallel settings the covering query planned serial Columnar Projection (projection-only). The same query with pgcolumnar.enable_projection_scan off planned Gather over a parallel base scan (gather-only).
  • The executor already partitions whatever storage BeginCustomScan opened (DSM stripe counter on readState). A partial covering path now carries the projection name, divides CPU the same way the parallel base path does, and keeps I/O undivided. After the change: Gather plus Columnar Projection: byik, count 181/181. I/O is still the base relation's pages; pricing from the projection's own storage pages is a separate defect.

Test plan

Independent twins test/projection_parallel.sh and test/pytest/test_projection_parallel.py. Same public seam (EXPLAIN of a covering query, plus count(*)). Different tables, row counts, stripes, bounds, and column names. Neither imports the other.

TDD on PG18, this session, before production:

Shell, unfixed .so:

-- parallel, projection on:
Custom Scan (PgColumnarScan) on cvppar
  Columnar Projection: byik
FAIL  a covering projection can be a parallel scan: got [projection-only] want [gather+projection]

Pytest, unfixed .so:

-- parallel, projection on: projection-only
AssertionError: a covering projection can be a parallel scan: got 'projection-only' want 'gather+projection'

After the partial covering path:

Shell: Gather + Columnar Projection: byik, count=181, 6 passed.
Pytest: gather+projection, count=400, 1 passed (6 assertions).

Causation (if (projName != NULL && 0) around the new add_partial_path): both twins red for the same got/want. Restored: both green. Fingerprint restored to 0c790f51a9e3.

Green on PG15, PG16, PG17, PG18, and PG19 (19beta2 on a sibling box). Ledger merged from those five logs plus the mutation red (--reds-are-real). Majors uniform 15;16;17;18;19. Census re-derived: awk -F'\t' '$5=="never"' -> 1382. suites_not_covered stayed 249. Collection: guard_tests 374 (unchanged), cluster_tests 419.

  • Independent twins red for the intended reason before production
  • Both green after the partial covering path
  • Causation mutation: both red for that same reason; restored green
  • Suite run on PG 15-19 and merged
  • I will not approve or merge this PR

Made with Cursor

@linuxhikerpm

Copy link
Copy Markdown
Author

TDD excerpts from this session. Prior chat summaries were not used as evidence. Start SHA 6ceb7dcb (v1.0-alpha4-45-g6ceb7dcb, origin/main after #1107). Confirmed in src/columnar_customscan.c on that tree: covering projection parallel_aware = false / parallel_safe = false; parallel base path custom_private = NIL.

Shell test/projection_parallel.sh (PG18)

Unfixed (no partial covering path):

-- serial:
Custom Scan (PgColumnarScan) on cvppar
  Columnar Projection: byik
-- parallel, projection off:
Gather
  Workers Planned: 4
  ->  Parallel Custom Scan (PgColumnarScan) on cvppar
-- parallel, projection on:
Custom Scan (PgColumnarScan) on cvppar
  Columnar Projection: byik
-- parallel covering count=181 want=181
FAIL  a covering projection can be a parallel scan: got [projection-only] want [gather+projection]

After add_partial_path of a covering CustomPath (parallel_aware = true, projection name in custom_private):

-- parallel, projection on:
Gather
  Workers Planned: 4
  ->  Parallel Custom Scan (PgColumnarScan) on cvppar
        Columnar Projection: byik
-- parallel covering count=181 want=181
accounting: 6 passed + 0 failed + 0 unrunnable + 0 skipped = 6

Causation (if (projName != NULL && 0)):

-- parallel, projection on:
Custom Scan (PgColumnarScan) on cvppar
  Columnar Projection: byik
FAIL  a covering projection can be a parallel scan: got [projection-only] want [gather+projection]

Restored: 6 passed. -- source: 0c790f51a9e3.

Pytest test/pytest/test_projection_parallel.py (PG18)

Unfixed:

-- serial: projection-only
-- parallel, projection off: gather-only
-- parallel, projection on: projection-only
-- parallel covering count=400 want=400
AssertionError: a covering projection can be a parallel scan: got 'projection-only' want 'gather+projection'

After the partial covering path:

-- parallel, projection on: gather+projection
-- parallel covering count=400 want=400
1 passed

Causation:

-- parallel, projection on: projection-only
AssertionError: a covering projection can be a parallel scan: got 'projection-only' want 'gather+projection'

Restored: 1 passed, 6 assertions.

Green on PG15, PG16, PG17, PG18, PG19 (19beta2). One merge of the five green logs (--expect-source 0c790f51a9e3) then the mutation FAIL with --reds-are-real. Census awk -F'\t' '$5=="never"' -> 1382. suites_not_covered stayed 249. Collection: guard_tests 374, cluster_tests 419.

I will not approve or merge this PR.

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things you have clearly taken on board since #1107 — both harness halves ship together, and the six ledger rows are seeded across all five majors. Neither needed saying this time.

One blocker, and it is the same rule that already has a precedent two directories away.

The test cannot see the defect it exists for

The two load-bearing arms are:

check "a covering projection can be a parallel scan"                shape == "gather+projection"
check "a parallel covering projection returns the covering rows once"  count == WANT

Both pass on a build where the partial path is offered but no worker ever claims a stripe. Gather is in the plan either way, and the leader alone produces exactly the right rows — so the count arm is satisfied by a scan that is parallel in name only. The pytest half asserts the same two things.

parallel_am_scan already does this properly, and it is the direct precedent:

parallel_am_scan.sh:93    Workers Launched: 2
parallel_am_scan.sh:105   "workers share the table-AM scan, it is not a single claimer"
                          # Sharing means both launched workers produced rows.
test_parallel_am_scan.py:114   (_first(analyzed, "Gather") or {}).get("Workers Launched")
test_parallel_am_scan.py:143   "workers share the table-AM scan, it is not a single claimer"

That suite exists because a first-wins phs_nallocated produced exactly this shape: a plan that looked parallel while one backend did all the reading. Your change routes the covering projection through the same shared counter, so it is exposed to the same failure and should carry the same assertions.

What I would add to each half: Workers Launched is 2, and both launched workers produced rows — per-worker, from EXPLAIN (ANALYZE, VERBOSE), not inferred from a total.

What I verified rather than took

Your comment claims the executor already partitions whatever storage BeginCustomScan opened, covering projection included. That holds structurally:

PgColumnarInitializeDSMCustomScan:  cstate->parallelCounter = counter;
                                    if (cstate->readState != NULL)
                                        PgColumnarReadSetParallelCounter(cstate->readState, counter);

The counter is attached to whatever readState is, so a projection's storage inherits it. The claim is sound — but it is exactly the claim the missing arms would demonstrate rather than argue.

Ledger and staleness, not a defect

keys added   6, all 15;16;17;18;19        <- correct, and a change from last time
keys "lost"  14                            <- NOT a deletion

The 14 are parts 400-a-check-result-must-be-machine and 530-a-record-must-name-its-major, which #1124 added after your branch's base. You are 4 commits behind 6ceb7dc and the PR shows CONFLICTING for the same reason. A rebase fixes both; nothing was removed.

Rebase locally and push — do not use Update branch. Your branch predates the union merge driver, so merging main in conflicts on CHANGELOG.md while rebasing onto main does not, because git reads .gitattributes from the tree being merged into. That asymmetry is now written up in CONTEXT.md.

Also worth keeping

I/O is still the base relation's pages, scaled by the same factor as the serial covering path. Pricing from the projection's own storage pages is a separate defect.

Naming the thing you did not fix, in the comment, at the place a reader will ask about it, is the right call. If that separate defect is not filed yet it is worth an issue so it does not live only in a code comment.

Happy to re-review as soon as the worker arms are in.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Static pass only so far — my box is finishing a two-major gate on another branch, and
running your suite alongside it would invalidate both. Two reseat items that main just
created under you, and one question about what the arm actually exercises.
I will
measure the parallel behaviour and post again.

First, the part worth saying plainly: every derived artefact in this PR is correct.

six new ledger rows      all majors=15;16;17;18;19        <- the #1107 defect, absent
census                   states 1382, re-derived 1382     <- agrees on your base
guard_tests              states 374,  collected 374
cluster_tests            states 419,  collected 419
TESTS.md                 section 52, no collision on your base
test_projection_parallel.py reads no shell source, so no SHELL_REFERENCES entry needed

That is all three of the defect classes the last two PRs tripped on, absent from the first
submission. Worth recording, since the reverse always gets recorded.

1. #1124 merged under you, and it moved both numbers

Main is now 3a741ad. Recomputed rather than guessed:

main census now              1391   (and main states 1391)
this branch states           1382
merged onto current main     1396   <- neither

The ledger auto-merges silently while the budget conflicts loudly, as usual. I checked the
silent half by key on the merge: 1408 rows, 0 duplicate keys, so it is a clean union
and only the count needs re-deriving.

2. Your TESTS.md section number now collides

#1124 landed test_record_names_its_major.py as 52, which is the number this branch
uses:

main:   ## 51. test_projection_scan_cost.py    ## 52. test_record_names_its_major.py
branch: ## 51. test_projection_scan_cost.py    ## 52. test_projection_parallel.py

Yours needs to become 53, in the heading and in its contents-list anchor. A naive
keep-both resolution produces a duplicate 52 — I checked by actually breaking the
document, and it is caught rather than shipped:

FAILED test_docs_cover_the_corpus.py::test_the_contents_list_is_numbered_in_order

3. What I went looking for and did not find

Recording the negative, because the shape of this change invited it. projName, projRun
and projScale are hoisted to function scope and consumed ~180 lines later in the parallel
block. That is a cross-block dependency, and the failure mode would be ugly: if any path
left projName non-NULL while skipping the projRun assignment, the parallel path would
compute ioRunProj = ioRun * projScale clamped to projRun = 0, then
cpuRunProj = 0 - 0, and offer a zero run cost path the planner must take.

It cannot happen. The serial block at 2915 is unconditional, runs before
if (rel->consider_parallel) at 3015, and everything between the projName != NULL guard
at 2921 and projRun = serialRun * projScale at 2977 is value clamping — no continue,
break, goto or return. So projName != NULL implies projRun was computed, and the
guard at 3155 is sufficient.

I would still rather that were expressed than inferred, since the next edit in between is
what breaks it. But it holds today.

And you did not duplicate the cost model, which is the thing I was most prepared to
find: the parallel path reuses projScale rather than recomputing sel. That matters
immediately — I have a fix in flight for #1126 that changes how sel is derived, and
because you reused the scale it will reach your parallel path with no second edit. Had you
copied the computation, one of us would have fixed one copy.

4. The question: does this arm exercise the partitioning?

LO=40  HI=220  WANT=$((HI - LO + 1))        # 181
SELECT count(*) FROM cvppar WHERE ik BETWEEN 40 AND 220

WANT being static arithmetic rather than derived from the path under test is right, and
parallel_leader_participation = off is right. But the projection is stored sorted on
ik with stripe_row_limit => 1000, so ik 40..220 is 181 contiguous rows inside the
first stripe
. Partitioning is per stripe. With four workers and one qualifying stripe,
the arm looks like it asserts "one worker read one stripe and the leader did not".

If that reading is right, a defect that mis-partitions across stripes is not covered, and
count(*) is the only oracle either way — a scan that read one stripe twice and skipped
another returns the right count whenever the two hold equally many matches.

Two cheap changes if you agree:

  • a range spanning many stripes, so more than one worker has something to do
  • pgc_set_hash instead of count(*) — it sorts before hashing so it is order-blind
    across workers, and 65 suites already use it, so it is the house oracle for exactly this

I am not asserting the single-stripe reading yet: it follows from the sort key and the
stripe limit, but I have not put EXPLAIN (ANALYZE, VERBOSE) on it to count the workers
that actually did work. That is the first thing I will run when the box frees, and I will
correct this section if the plan says otherwise.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Measured now that my box is free. Section 4 of my last comment was wrong — withdrawing
it.
The reseat items in sections 1 and 2 still stand.

Your arm does exercise the partitioning. I said it probably did not.

I reasoned that a projection sorted on ik with stripe_row_limit => 1000 puts
ik 40..220 inside one stripe, so four workers would have one stripe between them. That
was wrong: the projection is sorted per stripe, not globally, and the insert is
scrambled, so the 181 matches are spread across stripes. EXPLAIN (ANALYZE, VERBOSE):

Workers Planned: 4    Workers Launched: 4
  Worker 0:  rows=17
  Worker 1:  rows=155
  Worker 2:  rows=0
  Worker 3:  rows=9
                      17 + 155 + 0 + 9 = 181 = WANT

Three workers contributed. The count is only right if all three partitioned correctly, so
count(*) over this fixture is a real oracle and not the single-worker check I described.

That also weakens my pgc_set_hash suggestion enough that I would not hold anything for
it. Worth doing if you touch the file anyway, not worth a round trip.

Your suite passes here as submitted:

projection_parallel.sh on /usr/local/pg18a   6 passed + 0 failed, rc=0

And the thing I found while measuring is not yours

The parallel plan reports zero chunk groups:

Columnar Chunk Groups Total: 0
Columnar Chunk Groups Read:  0

That is not the projection path. The control, same query, same workers, projection off:

parallel + projection   Total 0    Read 0
parallel + BASE         Total 0    Read 0     <- pre-existing
serial   + projection   Total 32   Read 32    Vectors Skipped 32
serial   + base         Total 32   Read 32    Vectors Skipped 5

The counters are not accumulated from workers into the leader for any parallel columnar
scan, so EXPLAIN ANALYZE under-reports there today and this PR neither causes nor worsens
it. I mention it only so nobody reads that 0 in your new plan output and files it against
you. Happy to open a separate issue if you want it tracked; it is not a blocker for this.

The serial pair is a nice incidental confirmation that the projection is doing its job: 32
vectors skipped against the base's 5, on the same query.

Still outstanding, unchanged

  1. Census 1382 -> 1396 after test: eleven suites recorded every check against a major that is not a major (#1121) #1124 landed under you. Merged tree: 1408 rows, 0 duplicate
    keys, so the union is clean and only the count moves.
  2. TESTS.md section 52 -> 53, heading and contents-list anchor. test: eleven suites recorded every check against a major that is not a major (#1121) #1124 took 52.

Everything else I checked held on the first submission, which I said last time and is worth
repeating now that I have run it rather than read it.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Reseat items refreshed, because main moved twice more while this sat: #1134 landed at
21aa465 and #1129 at 17b2c4d
, so you are now 12 commits behind 6ceb7dcb and five
files conflict rather than the two I named earlier.

Before the steps, one finding that is worth more than the reseat, because it will bite the
arm @jdatcmd asked you for.

The precedent's assertion does not survive being copied to a 4-worker fixture

test_parallel_am_scan.py pins the worker count at 2 and then asserts every launched
worker produced rows:

expect.num((_first(analyzed, "Gather") or {}).get("Workers Launched"), 2, ...)
n_busy = sum(1 for r in worker_rows if r and r > 0)
expect.num(n_busy, 2, "workers share the table-AM scan, it is not a single claimer")

Your fixture launches 4. I measured it on your branch earlier and posted the numbers:

Workers Planned: 4    Workers Launched: 4
  Worker 0:  rows=17
  Worker 1:  rows=155
  Worker 2:  rows=0
  Worker 3:  rows=9

n_busy is 3 of 4. So the literal copy of the precedent fails on a build where your
change is working correctly — 181 matching rows spread over the stripes simply do not
reach every worker. Two ways out, and they are not equivalent:

  1. Pin max_parallel_workers_per_gather = 2, as the precedent does, and keep
    n_busy == launched. Strongest claim, and it matches the suite you are being asked to
    follow.
  2. Keep 4 workers and assert n_busy >= 2. The property in the name is "it is not a
    single claimer", and >= 2 is exactly that. Weaker, but honest, and it does not depend
    on the row distribution holding still.

I would take (1). Not because (2) is wrong, but because with 2 workers the assertion is
deterministic on the fixture you already have, and the defect this exists to catch — one
backend claiming every stripe — shows identically at 2 as at 4.

Either way, assert the per-worker rows from EXPLAIN (ANALYZE, VERBOSE) rather than a
total, which is the part @jdatcmd's review turns on: a total is satisfied by the leader
doing all of it.

The reseat, in the order you will do it

Rebase onto main; do not use Update branch. Merging main in conflicts on
CHANGELOG.md because git reads .gitattributes from the tree being merged into, and your
branch predates the union driver.

git fetch author main
git rebase author/main

Five files conflict, and all five are the shared anchors:

test/check_ledger.tsv              take BOTH sides' rows, then re-seed yours by RUNNING
test/check_ledger_budget.txt       re-derive, do not resolve
test/pytest/TESTS.md               main took section 56; yours becomes 57
test/pytest/expected_tests.txt     re-derive by collection
test/pytest/test_compare_to_bash.py    UNION the COMPLETE list

--ours and --theirs both drop entries on the last one. Main added
encode_post_codec to COMPLETE; you are adding projection_parallel. Taking either side
whole loses the other, the guard then reports the lost pair as undeclared, and the message
names your stem — which reads as your bug. I lost native_chunk_length_bound exactly
this way on #1093.

Your stated counts are stale by the amount main moved:

                 your branch     main now      after your rebase
guard_tests           374           380        re-derive
cluster_tests         419           423        re-derive

Do not add your delta to 423. Collect it:

cd test/pytest
G="$(python3 -c 'import sys; sys.path.insert(0,"."); from test_harness_deps import NO_CLUSTER; print(" ".join(NO_CLUSTER))')"
PYTHONPATH=. pytest --collect-only -q $G | tail -1          # guard half
# and the complement of $G for the cluster half

For TESTS.md, renumber your section to 57 and put it after main's 56 — after the
highest section main has, not where the conflict marker happens to sit. That distinction is
not pedantry: resolving in place is how #1095 came out numbered 47, 45, 46, and the numbers
looked fine in the diff.

What still stands from my earlier pass, unchanged

Every derived artefact in this PR was correct on its own base — six ledger rows across all
five majors, census 1382 stated and re-derived, TESTS.md complete. The reseat is arithmetic
against a moving main, not a defect in your work. And section 4 of my first comment stays
withdrawn: your arm does exercise the partitioning, and I was wrong about why it would not.

Ping me when the worker arms are in and I will measure them rather than read them.

@jdatcmd

jdatcmd commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Rebase target moved twice since my review, so aim at 17b2c4d, not 6ceb7dc: #1134 (post-codec encoding choice) landed as 21aa465, then #1129 as 17b2c4d. Neither touches columnar_customscan.c, so your change should rebase clean; both touch CHANGELOG.md and #1129 touches CONTEXT.md and two selftest parts, which is where the union driver earns its keep.

The blocker is unchanged: both load-bearing arms pass on a build where the partial path is offered and no worker ever claims a stripe. To save you deriving it, here is the shape parallel_am_scan uses, per worker rather than from a total:

EXPLAIN (ANALYZE, VERBOSE, COSTS OFF, TIMING OFF)
SELECT ... ;                      -- the covering-projection query

and then two assertions off that one plan:

  • Workers Launched: 2 on the Gather node — not Workers Planned, which is satisfied by a plan that launched none;
  • every launched worker produced rows, read from the per-worker actual rows lines under VERBOSE, not inferred by subtracting the leader's count from the total. A leader that did all the work and two workers that did none sums to the right total, which is exactly the shape the count arm cannot see.

One thing worth knowing before you re-run, because it will otherwise cost you an afternoon: enable_seqscan = off does not govern Custom Scan (PgColumnarScan). If an arm of yours needs a plan that is NOT the columnar custom scan, the switch is pgcolumnar.enable_custom_scan = off. I lost two rounds to that today in a suite of my own, and the arm that caught it was a premise asserting the plan node rather than the rows.

Also worth flagging for whenever you next touch the encoding descriptor: it is about to go v2 -> v3 (#1130), spending the header's reserved byte at offset 1 on a flags byte. Field offsets do not move and readers accept 2..3, so a byte-offset reader of entry fields is unaffected — but if you have anything that asserts the version byte is 2, it will move under you.

@OffgridwithJD
OffgridwithJD force-pushed the audit/covering-projection-parallel-path branch from cbf1182 to e81fdcf Compare September 19, 2026 00:21
@linuxhikerpm

Copy link
Copy Markdown
Author

@jdatcmd rebased locally onto current origin/main (not Update branch) and added the worker-share arms you asked for. Please re-review. I will not approve or merge this.

What you asked for

What I would add to each half: Workers Launched is 2, and both launched workers produced rows — per-worker, from EXPLAIN (ANALYZE, VERBOSE), not inferred from a total.

Independent twins now assert that, same names, own fixtures. Precedent is parallel_am_scan.

TDD (proved this session, PG18)

First-wins mutation of the shared group claim (pgcolumnar_next_group_index): Gather still present, covering count still exact, one launched worker produced rows.

Shell, mutated .so:

Workers Launched: 2
Worker 0:  actual rows=0.00
Worker 1:  actual rows=8000.00
-- worker rows: 0 8000  busy=1 lines=2
-- parallel covering count=8000 want=8000
FAIL  workers share the covering projection scan, it is not a single claimer: got [1] want [2]

Pytest, same mutation:

-- parallel, projection on: gather+projection
-- worker rows [0.0, 12100.0] busy=1 launched=2
-- parallel covering count=12100 want=12100
AssertionError: workers share the covering projection scan, it is not a single claimer: got 1 want 2

Restored the shared claim. Same assertion, both green.

Shell:

Workers Launched: 2
Worker 0:  actual rows=3981.00
Worker 1:  actual rows=4019.00
-- worker rows: 3981 4019  busy=2 lines=2
PASS  workers share the covering projection scan, it is not a single claimer

Pytest:

-- worker rows [6787.0, 5313.0] busy=2 launched=2
-- parallel covering count=12100 want=12100
1 passed

The short covering range (181 rows / two groups) was enough for Gather+count to pass while one worker finished the range. The twins now use 8000 / 12100 matching rows at the 100-row group floor, independently, so the share arm can see the claim. Measured green on 15, 16, 17, 18, and 19beta2.

HEAD is e81fdcf42931f89e274da249b0ba083eb0e77b11. Not merged, not self-approved.

@jdatcmd

jdatcmd commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

Re-read e81fdcf. The blocker is cleared, and the way you closed it is better than what I asked for.

I asked for Workers Launched: 2 and per-worker rows. You added a third arm between them that I did not ask for and should have:

check "premise: ANALYZE printed a rows= line per launched worker"   n_lines  2
check "workers share the covering projection scan, it is not a single claimer"  n_busy  2

with the reason in the file: "A worker that produced nothing still prints rows=0, so a missing line is not a zero — it is no measurement." Without it, n_busy counting 2 of 2 lines and n_busy counting 2 of 2 present lines are indistinguishable, and a parse that silently found nothing would read as a worker that did nothing. That is the distinction between an absent measurement and a measured zero, and it is the thing this class of arm usually gets wrong.

The four-arm shape now matches parallel_am_scan's, which is the precedent I cited, and it catches what the count arm cannot: Gather present, count right, one backend claiming every stripe.

One thing I checked and am NOT flagging

"$(echo "$par_on_ana" | grep -oE 'Workers Launched: [0-9]+' | head -1 | grep -oE '[0-9]+')"

That is the shape selftest/080 exists about, so I checked it rather than assuming either way. It is out of scope and safe on the merits: the rule is scoped to a reader whose exit status is the answer (grep -q), and here the answer is the pipeline's OUTPUT. head -1 may SIGPIPE the upstream grep, but the matched line has already been written and captured, so the value is right and only the discarded status is lost. The shipped detector's pattern agrees — it matches | grep -q and not | head.

I mention it because I broke that same rule twice in my own suite tonight, two functions below a header citing it, and harness_selftest caught me. Worth knowing where the boundary actually is rather than avoiding the shape by superstition.

What is left is mechanical

The branch is DIRTY against 51e24d0, which is four merges further on than your base. CI is green, but on a tree that no longer exists.

Rebase locally and push — do not use Update branch, for the same reason as last time: your branch predates the union merge driver that CHANGELOG.md needs, and a merge commit will take the wrong side of it.

Re-review as soon as it is green on a current base. Nothing else outstanding from me.

@linuxhikerpm
linuxhikerpm force-pushed the audit/covering-projection-parallel-path branch from e81fdcf to c4d8d2d Compare September 19, 2026 17:59
@linuxhikerpm

Copy link
Copy Markdown
Author

@jdatcmd rebased locally onto current origin/main 51e24d07c294 (not Update branch) and force-with-lease pushed. New head c4d8d2da28dbc819b5ae413e758be7a5f4695336. I will not approve or merge this.

What you asked for

The branch is DIRTY against 51e24d0, which is four merges further on than your base. CI is green, but on a tree that no longer exists.

Rebase locally and push — do not use Update branch, for the same reason as last time: your branch predates the union merge driver that CHANGELOG.md needs, and a merge commit will take the wrong side of it.

Re-review as soon as it is green on a current base. Nothing else outstanding from me.

Replay was d88be96 (production) then 616740d (worker-share arms). The only conflict was test/check_ledger_budget.txt. Kept main's validity-elision census comment and re-derived checks_never_observed_red by counting never on the rebased ledger: 1428 (main 1421 plus seven never projection_parallel rows; two worker-share last-red rows stay last-red). Union driver duplicated the Unreleased CHANGELOG note; dropped the second copy. cluster_tests re-derived by collection: 436 (was 435 on the pre-reseat tree). guard_tests stayed 382. suites_not_covered stayed 249.

TDD on the reseated tree (PG18, this session)

Shell test/projection_parallel.sh after rebuild:

-- parallel, projection on:
Gather
  Workers Planned: 2
  ->  Parallel Custom Scan (PgColumnarScan) on cvppar
        Columnar Projection: byik
-- worker rows: 4797 3203  busy=2 lines=2
-- parallel covering count=8000 want=8000
PASS  a covering projection can be a parallel scan
PASS  a parallel covering projection returns the covering rows once
PASS  premise: EXPLAIN ANALYZE launched two workers
PASS  premise: ANALYZE printed a rows= line per launched worker
PASS  workers share the covering projection scan, it is not a single claimer
projection_parallel.sh: PASSED

Pytest test/pytest/test_projection_parallel.py:

checks run: 9
accounting: 9 pass + 0 fail + 0 unrun = 9
1 passed in 1.58s

Not merged. Not self-approved.

@jdatcmd

jdatcmd commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

Your blocker is cleared and the work verifies. main moved underneath you three more times while you were rebasing, so rather than send you round again I composed your c4d8d2da onto current main here, resolved it, and ran it. Here are the exact numbers, so your rebase is mechanical.

Composed onto 7a1095f9 (main carrying #1152, #1156, #1147, all merged in the last two hours).

What conflicts, and how I resolved it

Three files, and the fourth is the one worth noticing.

test/pytest/TESTS.md             CONFLICT   your section takes 70, not 66
test/check_ledger_budget.txt     CONFLICT   both census values stale
test/pytest/expected_tests.txt   CONFLICT   both cluster_tests values stale
test/check_ledger.tsv            AUTO-MERGED SILENTLY   <- the quiet one

The ledger auto-merged without a word while the budget conflicted loudly. That is the usual asymmetry and it held again: the file that is always right stays quiet, the file that always speaks is always wrong. I re-counted the ledger rather than trusting the silence.

The numbers, derived on the composed tree

TESTS.md section                 70   (66 temporal, 67 parquet-oob, 68 advisory-lock, 69 index-am)
                                      heading AND contents entry, both
checks_never_observed_red      1446   awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
cluster_tests                   442   by collection
guard_tests                     382   by collection, did not move
suites_not_covered              249   did not move

Neither side's number survived, either time. Main said 1439 and you said 1428; the tree counts 1446. Main said 440 and you said 436; the tree collects 442. Your 436 and 1428 were correctly derived against 51e24d0 — they are right for a tree that no longer exists, which is the thing this repository keeps paying for. 1439 + 7 and 1428 + 18 both happen to reach 1446, and that is a coincidence of this merge rather than a method.

Verified on the composed tree, PG 17

projection_parallel.sh              9 passed + 0 failed + 0 unrunnable
test_projection_parallel.py         9 pass + 0 fail + 0 unrun
compare_to_bash.py                  9 literal, 0 template, missing 0 -- every property covered
harness_selftest.sh                 1081 passed + 0 failed
pytest cluster leg                  442 passed, want 442
pytest guard leg                    382 passed, want 382

A caveat about two of those numbers, because I got them wrong first. My initial runs reported harness_selftest at 1062 + 19 failed and one cluster-leg failure. Both were my instrument, not your branch: I had shipped the tree into the container without a usable .git, and nineteen arms said got [no-repo] want [ignored] while test_pgxn_metadata failed on git archive. Re-run from a real git clone, it is 1081 + 0 and 442 + 0. I mention it because a tree copied without .git is a documented false red here and it still caught me three times today.

What to do

Rebase onto current main locally — not Update branch, for the CHANGELOG.md union-driver reason you already know. You will hit the same three conflicts. Take section 70, then re-derive both numbers yourself rather than copying mine:

awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
cd test/pytest && PYTHONPATH=. pytest --collect-only -q --pg-config <pg_config> $F | tail -1

They should land on 1446 and 442. If they do not, yours is right and mine is stale — main may well move again before you push, and the recipe is the authority, not my figures.

The review itself

Your reply to my earlier round did everything asked: rebased locally rather than with Update branch, re-derived rather than added, dropped the union driver's duplicate CHANGELOG line, and kept the two worker-share rows as last-red rather than resetting them. The TDD output shows the plan asserted on both arms, which is what makes the parallel claim mean anything.

Re-approving as soon as it is green on a current base. Nothing else outstanding from me.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

@jdatcmd

jdatcmd commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

Superseding the numbers I gave you two comments ago. main has taken #1160 and #1161 since, so my figures are stale — which is the thing I warned about and it happened within the hour.

main is now e7eafcfe and states:

TESTS.md                70 numbered sections   (70 is test_analyze_differential.py, #1161)
cluster_tests          442
guard_tests            382
checks_never_observed_red   1439   (counted: 1439 -- #1161 adds no ledger rows)
suites_not_covered     249

So your section is 71, not 70. My earlier cluster_tests 442 and checks_never_observed_red 1446 were derived against 7a1095f9 and describe a tree that no longer exists.

I am deliberately not giving you replacement figures. I gave you numbers last time and they went stale before you could use them; the recipe is the authority and it is the only thing that does not rot:

awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
cd test/pytest && PYTHONPATH=. pytest --collect-only -q --pg-config <pg_config> $F | tail -1

Everything I verified about your branch still stands — the composition is clean, the conflicts are the same three files, and on the tree I built it ran:

projection_parallel.sh        9 passed + 0 failed
test_projection_parallel.py   9 pass + 0 fail
harness_selftest.sh           1081 passed + 0 failed

Only the counts moved, and counts are the part that is meant to be re-derived rather than carried. Nothing about your code or your tests needs to change.

If main moves again before you push, take the same approach: rebase, resolve the same three files, derive both numbers, take the next free section number. Ping me when it is up and I will re-review immediately — the churn here is mine and the peer's landing work, not you being slow, and I would rather you did not pay for it a fourth time.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Not a blocker, and not a request to change anything yet — an offer to measure one thing,
from someone who has spent today being wrong about exactly this failure mode.

The arm addressing @jdatcmd's blocker is the right arm

n_busy == 2 over Worker N:.*rows= is the correct observable, and the premise beside it
is what makes it safe:

check "premise: ANALYZE printed a rows= line per launched worker"  "$n_lines" "2"
check "workers share the covering projection scan, it is not a single claimer"  "$n_busy" "2"

A worker that produced nothing still prints rows=0, so a missing line is not a zero — it
is no measurement, and the n_lines premise catches that rather than letting n_busy read
an absent worker as an idle one. awk '$1>0{n++} END{print n+0}' on empty input gives 0
rather than blank, so both arms fail closed. That is the same shape parallel_am_scan
uses and the precedent @jdatcmd pointed at.

The fixture work behind it is visible and good: parallel_leader_participation = off so
the leader cannot absorb the range, ~80 groups at chunk_group_row_limit => 100, and
repeat(md5(...), 12) so each claimed group carries real decode work. The header records
that a two-int projection let one worker claim everything before the other started, which
is the failure this fixture was rebuilt to avoid.

The one question: how many times has it run?

The header says the geometry is "what kept both workers busy on every major this run
measured
". That reads as one run per major.

n_busy == 2 is a claim about the SCHEDULER, not about the code under test. It is true
whenever both workers get at least one of ~80 groups, which should be nearly always with
leader participation off — but "nearly always" is a rate, and a rate measured once is not
measured. The repository's own position is that a guard with a bad false-positive rate gets
switched off, and then the guard it replaced is gone too.

Why I am raising it rather than assuming it is fine. I hit a single red today in
test_sorted_pathkeys.py on a PG15 leg, and four experiments later — the arm alone at
N=50, the full corpus at N=20, an A/B at N=10 per arm — every condition has come back
clean and the original observation is still unexplained. One run is not evidence of a
rate in either direction, which is the lesson, and it applies to a green arm exactly as
much as to a red one.

The offer

When the box frees up I can run this suite N=20 on PG15 and PG18 and report n_busy and
n_lines per round, raw. If it is 20/20 on both, that bounds the flake rate under ~15%
and the question is closed with a number in the header instead of a run count of one. If
it is not, you would want to know before it lands rather than from a nightly.

Say the word and I will queue it; it is about twenty minutes and it costs you nothing.

One thing already correct that I want to name

Both halves ship together and neither reads the other, and the pytest half derives worker
rows from FORMAT JSON (scan["Workers"] → Actual Rows) while the shell half greps the
text form. Same property, two unrelated parsers. That is the independence rule working
rather than being recited.

@jdatcmd

jdatcmd commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

The base has stopped moving, and we are holding it that way for you.

main is still e7eafcf — unchanged since my last comment — and @OffgridwithJD has paused landing the port queue specifically so this can go in against a stable base. The figures I gave you are therefore still current, re-derived just now on origin/main:

TESTS.md         70 numbered sections   -> your section is 71
cluster_tests   442
guard_tests     382
census         1439

Rebase now and they will hold. Still derive rather than copy — the recipe is the authority and I have had two sets go stale on you today — but you should find them unchanged this time.

For what it is worth, the churn was ours: five merges in about two hours while you were rebasing, and the numbers you derived correctly against 51e24d0 were invalidated twice through no fault of your work. That is the cost of a fast-moving base landing on the contributor who is not driving it, and pausing the queue is the right correction.

Nothing has changed in my assessment. When I composed your c4d8d2da onto main earlier it ran clean:

projection_parallel.sh        9 passed + 0 failed + 0 unrunnable
test_projection_parallel.py   9 pass + 0 fail + 0 unrun
harness_selftest.sh           1081 passed + 0 failed

so the only thing between this and a merge is the rebase and the three numbers. Ping me when it is pushed and I will re-review straight away.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

@linuxhikerpm
linuxhikerpm force-pushed the audit/covering-projection-parallel-path branch from c4d8d2d to 43dc442 Compare September 21, 2026 23:06
@linuxhikerpm

Copy link
Copy Markdown
Author

@jdatcmd rebased locally onto current origin/main c2859e43 (Merge #1175; not Update branch) and force-with-lease pushed. New head 43dc44255f05e4ec7abf9fa1c3200832c11e4f19. I will not approve or merge this.

What you asked for

Rebase onto current main locally — not Update branch. [...] Your section is 71, not 70.

Main moved again after that comment (now c2859e43, TESTS.md already through 73). Replay was d88be96 (production) then 616740d (worker-share arms). The stale reseat-only number commits were not replayed.

Derived on this tree, not copied

TESTS.md section                 74
checks_never_observed_red      1453   awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
cluster_tests                   463   by collection (`463 tests collected`)
guard_tests                     393   by collection, did not move
suites_not_covered              249   did not move

Conflicts: TESTS.md (section 74 after main's 73), then check_ledger_budget.txt (re-counted). Union driver duplicated the Unreleased covering note; dropped the second copy. COMPLETE and SUITES auto-merged C-sorted.

Green on the reseated tree (PG18, this session)

Shell test/projection_parallel.sh:

Workers Launched: 2
Worker 0:  actual rows=2265.00
Worker 1:  actual rows=5735.00
-- worker rows: 2265 5735  busy=2 lines=2
-- parallel covering count=8000 want=8000
PASS  a covering projection can be a parallel scan
PASS  a parallel covering projection returns the covering rows once
PASS  premise: EXPLAIN ANALYZE launched two workers
PASS  premise: ANALYZE printed a rows= line per launched worker
PASS  workers share the covering projection scan, it is not a single claimer
accounting: 9 passed + 0 failed + 0 unrunnable + 0 skipped = 9

Pytest test/pytest/test_projection_parallel.py:

checks run: 9
accounting: 9 pass + 0 fail + 0 unrun = 9
1 passed in 1.44s

Please re-review. Not merged. Not self-approved.

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial re-review of 43dc442, built and run in the audit container on PG 18.4 (assert build). I attacked the cost model, the test's sensitivity and the interaction with #1155. The suite held. One latent trap remains, and it is keyed to the PR next to this one.

The attack that failed, which is the point

The fixture sets parallel_setup_cost = 0 and parallel_tuple_cost = 0. That removes the penalty a badly-priced partial path would normally pay, so I expected the arms to pass no matter what the new arithmetic computed. I was wrong.

I priced the partial covering path exactly like the serial covering path, deleting every parallel advantage the cost model grants:

-				prpath->path.total_cost = serialStartupCost +
-					ioRunProj + cpuRunProj / divisor;
+				prpath->path.total_cost = serialStartupCost + projRun;

Rebuilt (.so ee8d36604e9a, source restored and git status clean afterwards):

-- worker rows:   busy=0 lines=0
FAIL  a covering projection can be a parallel scan: got [projection-only] want [gather+projection]
FAIL  premise: EXPLAIN ANALYZE launched two workers: got [] want [2]
FAIL  premise: ANALYZE printed a rows= line per launched worker: got [0] want [2]
FAIL  workers share the covering projection scan, it is not a single claimer: got [0] want [2]
accounting: 5 passed + 4 failed + 0 unrunnable + 0 skipped = 9

Even with the Gather penalty zeroed, a partial path priced level with the serial one loses, and four arms say so. The cost arithmetic is pinned, not merely exercised. That is more than I could say for #1155's arms, which survived the equivalent mutation unchanged.

The three worker rows arms are the good part of this suite. They are the difference between "Gather appeared in the plan" and "two workers each returned rows from the shared claim", and they are what made the mutation above visible as four failures rather than one.

The finding: a dead branch that #1155 brings to life

ioRunProj = ioRun * projScale;
if (ioRunProj > projRun)
    ioRunProj = projRun;
cpuRunProj = projRun - ioRunProj;

That clamp cannot fire on this branch. Line 3178 already did the same clamp one level up:

ioRun = pgcolumnar_scan_io_run_cost(rel, rte->relid);
if (ioRun > serialRun)
    ioRun = serialRun;

so ioRun <= serialRun, and projRun is serialRun * projScale with projScale in [0, 1]. Multiplying both sides by a non-negative projScale preserves the order, so ioRunProj <= projRun always. The branch is unreachable, which is harmless.

It stops being unreachable the moment projRun stops being serialRun * projScale — which is exactly what #1155 does, replacing it with cpuRun * scale + ioProj. And the state the clamp produces when it binds fully is not neutral. With ioRunProj == projRun, cpuRunProj is zero and the total becomes

serialStartupCost + projRun

which is character-for-character the serial covering path's total. That is the mutation I ran above, the one that turned four arms red and lost the feature. So the clamp's binding case and my mutation are the same state, and I have measured what it costs.

One narrower case is already reachable on this branch alone: if line 3178's clamp binds, ioRun == serialRun, so ioRunProj == projRun and cpuRunProj is zero by the same route. That also zeroes cpuRun for the base partial path, so it is a degeneracy the parallel base scan shares rather than something this PR introduces. Worth knowing it is there.

What I would want: a comment saying what the clamp is for and under what condition it may bind, or an assertion that it does not. Right now a reader cannot tell it is dead, and the next person to touch projRun will not know they have armed it.

Measured, not assumed: I grafted #1155's projRun computation onto this branch by hand, rebuilt (.so ce23b984ac7a) and ran your suite. It stayed green:

accounting: 9 passed + 0 failed + 0 unrunnable + 0 skipped = 9

So the two PRs do not collide on this fixture. I had predicted they would, and they did not. The clamp does not bind here because the fixture runs at default costs, where the CPU term dominates and there is plenty left to divide. It is the I/O-heavy shape, the one #1155's own fixture builds with seq_page_cost = 1000 and the CPU costs zeroed, where the margin disappears.

Note also that the two branches conflict textually in PgColumnarSetRelPathlist: this PR renames scale to projScale and hoists projRun to function scope, and #1155 replaces the line that assignment sits on. Whoever rebases second resolves it by hand, and the resolution is where the clamp gets armed.

A smaller question

The serial covering path sets parallel_safe = false; the new partial path for the same scan of the same projection sets parallel_safe = true. A partial path must be marked safe, so the new line is forced. That makes the older line the one worth asking about: if this scan is genuinely parallel-safe, the serial path is under-marked and cannot be used inside any parallel plan. I have not proved a plan that suffers for it, so treat this as a question rather than a finding.

What I could not fault

  • projRun cannot reach the new block at its initializer. projName != NULL implies the assignment ran, with no early exit between them.
  • The new block sits inside if (rel->consider_parallel), so it inherits the parallel-safety checks rather than restating them.
  • The suite's own result on its own head: 9 passed + 0 failed, .so ddffa24ed2d7.

No objection to the approach, and the test is stronger than most. Add the comment on the clamp and say how you want the #1155 overlap resolved.

Container pgcolumnar-audit, PG 18.4 assert build. Every mutation restored and verified clean.

@linuxhikerpm

Copy link
Copy Markdown
Author

@OffgridwithJD @jdatcmd rebased locally onto current origin/main e9885d3d (not Update branch). New head below. I will not approve or merge this.

What you asked for

a comment saying what the clamp is for and under what condition it may bind

Documented at the ioRunProj clamp: unreachable while projRun = serialRun * projScale; live if projRun is computed independently (e.g. from projection pages); full bind zeroes cpuRunProj and makes the partial path total like the serial covering path.

Green on the reseated tree (PG18): shell 9 passed (Workers Launched busy=2, rows 3967/4033); pytest 9 pass. Re-derived: guard_tests 398, cluster_tests 464, checks_never_observed_red 1458.

Please re-review. Not merged. Not self-approved.

@linuxhikerpm
linuxhikerpm force-pushed the audit/covering-projection-parallel-path branch from 43dc442 to 64a3935 Compare September 22, 2026 17:10

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of 64a3935. The comment is accurate, and it says the three things that matter rather than only the first.

/*
 * Clamp ioRunProj to projRun. With projRun = serialRun *
 * projScale this is unreachable: ioRun was already clamped
 * to serialRun one level up, and multiplying both sides by
 * the same non-negative projScale preserves the order. It
 * becomes live if projRun is ever computed independently
 * (for example from the projection's own pages). When the
 * clamp binds fully, cpuRunProj is zero and the partial
 * covering path totals exactly like the serial covering
 * path, so Gather loses.
 */

It states that the branch is dead today, why it is dead (the clamp one level up plus order preservation under a non-negative multiplier, which is the part a reader cannot reconstruct from the two lines), what arms it — #1155 computing projRun from the projection's own pages is precisely the "independently" case — and what happens when it binds, which is the consequence I measured: pricing the partial covering path level with the serial one takes the suite to 5 passed + 4 failed and the feature disappears.

That is what I asked for. A reader touching projRun now knows they are arming something.

Worth noting for whoever rebases second: #1155 is the change that makes this live, and its current head computes projRun as cpuRun * scale + ioProj with ioProj derived from the projection's own rounded pages. On its fixture that leaves the CPU term dominant, and I measured the two together earlier: grafting #1155's computation onto this branch left your suite at 9 passed + 0 failed. So the clamp does not bind there. The shape where it would is the I/O-heavy one, and the comment now names it.

Nothing else from me. Holding the approval only because mergeStateStatus=DIRTY and the head will move: #1191 and #1193 merged under this branch. Rebase and I will approve on the new head — I am not placing a sign-off that would survive onto commits I have not read.

Verified at 64a3935.

@OffgridwithJD
OffgridwithJD force-pushed the audit/covering-projection-parallel-path branch from 64a3935 to 6f605f3 Compare September 22, 2026 18:17
@linuxhikerpm

Copy link
Copy Markdown
Author

@OffgridwithJD @jdatcmd rebased locally onto current origin/main 133c3fbd (not Update branch). New head 6f605f38c1a6f70f27857c2d8f2eee0fed909425. I will not approve or merge this.

What you asked for

@OffgridwithJD (adversarial review of 43dc442):

What I would want: a comment saying what the clamp is for and under what condition it may bind

That comment was already on prior head 64a39355 and remains after this rebase (at the ioRunProj clamp: unreachable while projRun = serialRun * projScale; live if projRun is computed independently; full bind zeroes cpuRunProj).

@jdatcmd earlier blocker (Workers Launched / both workers produce rows) was cleared on prior heads and still holds — measured again below.

Derived on this tree, not copied

checks_never_observed_red      1460   awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
cluster_tests                   464   by collection
guard_tests                     398   by collection
suites_not_covered              249   did not move
TESTS.md section                 74   (contents list matches headings)

Green on the reseated tree (PG18, this session)

Shell:

Worker 0:  actual rows=4018.00
Worker 1:  actual rows=3982.00
-- worker rows: 4018 3982  busy=2 lines=2
-- parallel covering count=8000 want=8000
PASS  workers share the covering projection scan, it is not a single claimer
accounting: 9 passed + 0 failed + 0 unrunnable + 0 skipped = 9
projection_parallel.sh: PASSED

Pytest:

checks run: 9
accounting: 9 pass + 0 fail + 0 unrun = 9
1 passed in 1.74s

reviewDecision is still CHANGES_REQUESTED only because a re-review has not landed on a current base — the clamp comment and worker arms already match the outstanding requests at this SHA.

Please re-review. Not merged. Not self-approved.

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 6f605f3. My blocker was that the two load-bearing arms passed on a build where no worker ever claimed a stripe. Both halves now assert two workers launched and both producing rows, per worker from EXPLAIN (ANALYZE, VERBOSE), which is the same seam parallel_am_scan uses. That is what I asked for.

I ran it rather than read it

PGC_SKIP_BUILD=1 test/projection_parallel.sh, PG 18.4, container pgcolumnar-dev

  idle box                        20 reps   20 passed   0 failed
  8 busy loops on 8 cores         12 reps   12 passed   0 failed
                                  ------------------------------
                                  32 reps   32 passed   0 failed

Why I loaded the box. "both launched workers produced rows" is a scheduling property, and an idle machine is the case most likely to satisfy it. This repository has a measured precedent in the other direction: a race that was 0 of 400 idle and 6 of 400 under load. Twenty green idle reps would not have told me the arm is stable. Thirty-two across both conditions does.

Your header already records that you hit this: 181 rows over two groups and 2000 over twenty both let one worker finish before the other claimed, and 8000 rows over eighty groups with the repeat(md5(...), 12) decode work is what kept both busy. Writing down the geometries that did NOT work, and why, is worth more than the one that did, and it is the reason I trusted the fixture enough to spend the reps on it.

parallel_leader_participation = off with parallel_workers = 2 is the right pairing. Four workers on this fixture could leave one idle and turn the arm into a test of scheduling, which your comment says.

One thing that is not yours, and is live right now

cluster_tests 464 will merge silently and be wrong. #1180 merged a few minutes ago and took main from 463 to 464. Your branch says 464 as well. Same value on both sides, so git does not speak, and the composed tree has both new tests.

I proved this on the neighbouring PR rather than predicting it. Composing current main (b986d8d) with #1155, which also says 464:

the merged file says     cluster_tests 464
the composed tree has    47 cluster files
collection reports       465 tests collected

And the part that makes it worse than a plain silent merge: expected_tests.txt DID conflict, in the COMMENT block at lines 473-482. The value at line 483 sat outside the conflict and auto-merged at 464. So the conflict draws a reader to the prose and away from the number.

Your checks_never_observed_red 1460 will conflict loudly against main's 1461, which is the safe half, and re-deriving that one is already in your habits.

So: rebase onto b986d8d and re-derive cluster_tests by collection before this merges. Expect 465. I am approving rather than blocking because the arithmetic is not the change under review and the fix is one command on a rebased tree.

The substance I re-checked

claim how
the worker arms are per-worker, not a total _worker_rows walks plan["Workers"] and reads each Actual Rows
the shell half does the same independently its own ANALYZE parse, not the python one
the premises are real a rows= line per launched worker, asserted before the sharing arm reads them
the counter reaches a projection's storage PgColumnarInitializeDSMCustomScan attaches it to whatever readState is, which I checked on the first pass and is unchanged

CI is 15 of 15 with nothing failing.

@jdatcmd

jdatcmd commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

My approval stands, and it now rests on a composed run rather than on reasoning. I withdrew my approval of #1155 an hour ago because it is green alone and red composed with main, so I owed the same check here rather than assuming.

main at c4f1c51, which carries #1180, merged with this branch. Conflicts are bookkeeping only, zero markers left, and both changes asserted present in the tree by name before anything was measured.

test/projection_parallel.sh   9 passed + 0 failed
  PASS  premise: a serial covering query uses the projection
  PASS  a covering projection can be a parallel scan
  PASS  premise: EXPLAIN ANALYZE launched two workers
  PASS  workers share the covering projection scan, it is not a single claimer

test/base_scan_io.sh          9 passed + 0 failed
  -- before_run=22000 after_run=22000 ratio=1.000

Both directions: your suite passes with #1180 present, and #1180's own suite passes with your change present.

Why this was worth the run rather than an argument

#1180 changed rel->pages, and three of your arms are plan-choice assertions -- whether the planner picks the covering projection, and whether it offers a partial path. A cheaper base scan is exactly the thing that could flip a plan choice, and the diff would not show it because your code does not mention rel->pages at all.

That is the shape that caught #1155: no file in common, no shared line, no cross-file grep would find it. The coupling there was a catalog FIELD -- their suite clears proj_storage_id and #1180's new sibling walk reads it -- so their oracle and their arm were read in two different catalog states. Yours does not mutate the catalog and the plan choices hold.

The cluster_tests line still needs re-deriving, and it has moved again

main now says 466, after #1196 landed. Your branch says 464 and conflicts with main on four files, so you have to rebase anyway. Derive it by collection on the rebased tree, and read pytest's own N tests collected line rather than counting matches in the listing: a diagnostic in that output NAMES the tests it warns about, which inflates a grep -c by exactly one and reads like a consistent census.

@jdatcmd

jdatcmd commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Approved and green, but not mergeable: CONFLICTING against main at 3519856e. Four PRs landed after your last rebase (#1195, #1196, #1199, #1200), so this needs one more. I composed it and derived every number the rebase has to produce, so the rebase is mechanical rather than a hunt.

What conflicts, and what does not

UU  test/check_ledger_budget.txt
UU  test/pytest/TESTS.md
UU  test/pytest/expected_tests.txt
    test/check_ledger.tsv        auto-merged SILENTLY this time

Nothing in src/ and nothing in your suite. check_ledger.tsv conflicted on the previous main and does not on this one, which is the positional silence: whether git speaks depends on how far apart the suite names sort, not on whether the merge is safe. I checked it rather than trusting it -- 1526 rows, 9 for projection_parallel, zero duplicate (suite, part, check) keys.

The four numbers, derived by collection on the composed tree

key your branch main the compose
guard_tests 398 402 402
cluster_tests 464 466 467
checks_never_observed_red 1460 1492 1499
suites_not_covered 249 249 249

cluster_tests is the one to be careful with: 467 is main's 466 plus your one new test file, over 48 cluster files. It is not 466 and it is not your 464, and a three-way merge will not produce it for you.

Read pytest's own N tests collected line, not a count of the listing. --collect-only -q prints a vacuity diagnostic that NAMES the tests it warns about, so grep -cE '::test_' comes back +1 on every tree. That cost an afternoon here today: it read as a consistent census and produced a false alarm that main was shipping a stale number.

TESTS.md needs a renumber, not a merge

Your section is 74, and main now has 74, 75 and 76:

## 74. test_base_scan_io.py: ...             (#1180)
## 75. test_range_pruning.py: ...            (#1196)
## 76. test_docs_upgrade_chain.py: ...       (#1199)

So yours becomes 77, in the heading, the TOC entry and the anchor slug. Derive it as max(existing) + 1 rather than typing it; both @OffgridwithJD and I hardcoded a number today and both were wrong within the hour.

Rebuild the file from main and re-apply your section once, so the diff carries zero deletions. Then check it as a pairing, not as two counts: sections and TOC entries agreeing on number and title, numbers contiguous, every anchor matching its heading slug. sections == contents passes while two sections share a number.

The behaviour is verified against this main

Nothing here is a doubt about the change. Composed with main and run:

test/projection_parallel.sh   9 passed + 0 failed
test/base_scan_io.sh          9 passed + 0 failed   (-- ratio=1.000)

Both directions, and @OffgridwithJD composed it independently and got 9 passed with a different worker split (2276/5724), which is better corroboration than matching numbers would have been.

My approval stands. Rebase, re-derive those four values on the rebased tree rather than copying them from this comment, and I will merge it.

The covering projection path was serial-only, so it could not compete
with a parallel base scan: either Gather dropped the projection or the
serial projection dropped the workers. The executor already partitions
whatever storage BeginCustomScan opened.

Co-authored-by: Cursor <cursoragent@cursor.com>
OffgridwithJD and others added 4 commits September 22, 2026 16:48
Gather and the covering count still passed when one worker claimed
every stripe. EXPLAIN ANALYZE now requires Workers Launched is 2 and
both launched workers produced rows, matching parallel_am_scan.

Co-authored-by: Cursor <cursoragent@cursor.com>
The clamp is dead while projRun is serialRun * projScale; it becomes live
if projRun is computed independently. Re-derive guard_tests and the never
census after rebase onto current main.

Co-authored-by: Cursor <cursoragent@cursor.com>
Main moved past e9885d3 (commandprompt#1193). Re-derive checks_never_observed_red by
counting field 5; confirm guard/cluster by collection.

Co-authored-by: Cursor <cursoragent@cursor.com>
Rebased onto e2638b7. Four PRs landed after this branch's previous rebase,
so every tracked number it carried was derived against a main that no longer
exists.

RE-DERIVED BY MEASUREMENT ON THE COMPOSED TREE, never by keeping either side
of a conflict and never by adding this branch's delta to main's value:

    cluster_tests              464 -> 468   collection, 48 cluster files
    guard_tests                398 -> 403   main's, untouched by this branch
    checks_never_observed_red 1460 -> 1499   awk over the ledger
    suites_not_covered         249          unchanged

Neither side's number was the composed one and their difference was not the
delta, which is why each was measured rather than reconciled.

check_ledger.tsv auto-merged without a conflict, so it was checked rather than
trusted: 1526 rows, zero duplicate (suite, part, check) keys.

TESTS.md is rebuilt from main with this branch's section re-applied once, so
the diff carries no deletions. The section number is DERIVED as max + 1 rather
than kept: main's highest is now 76, so test_projection_parallel.py is 77 and
not the 74 it was written as. Checked as a pairing rather than as two counts:
77 sections, 77 TOC entries, 77 of 77 agreeing on number AND title, contiguous
1..77, zero bad anchors.

Verified on the rebased tree:

    projection_parallel.sh        9 checks, PASSED
    test_projection_parallel.py   9 checks, 1 passed
    the corpus guards           300 checks, 80 passed

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
@jdatcmd
jdatcmd force-pushed the audit/covering-projection-parallel-path branch from 6f605f3 to 44c2f5c Compare September 22, 2026 22:49
@jdatcmd

jdatcmd commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Pushed a rebase onto e2638b78 to this branch, at the owner's request. No behaviour changed; everything here is bookkeeping that went stale while the branch sat.

Four PRs landed after your last rebase, so every tracked number was derived against a main that no longer exists.

was now how
cluster_tests 464 468 collection on the composed tree, 48 cluster files
guard_tests 398 403 main's, untouched by this branch
checks_never_observed_red 1460 1499 awk -F'\t' '$5=="never"'
suites_not_covered 249 249 unchanged

Each was measured on the composed tree, not kept from either side of a conflict and not obtained by adding your delta to main's value. Neither side's number was the composed one and their difference was not the delta.

check_ledger.tsv auto-merged without a conflict, which is the positional silence rather than a safety guarantee, so it was checked: 1526 rows, zero duplicate (suite, part, check) keys.

TESTS.md is rebuilt from main with your section re-applied once, so the diff carries no deletions, and the number is derived as max + 1: main's highest is now 76, so test_projection_parallel.py is 77 rather than the 74 it was written as. Checked as a pairing rather than as two counts -- 77 sections, 77 TOC entries, 77 of 77 agreeing on number and title, contiguous, zero bad anchors.

Verified on the rebased tree:

projection_parallel.sh        9 checks, PASSED
test_projection_parallel.py   9 checks, 1 passed
the corpus guards           300 checks, 80 passed

My approval from earlier stands and the composed run that earned it is unchanged. @OffgridwithJD, the numbers are the part worth a second pair of eyes since I derived them and I am also the one approving.

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 44c2f5c. 15 checks, all SUCCESS, CLEAN, behind_main=0.

@jdatcmd pushed the rebase here and is also clearing the blocks on the sibling PRs, so this is the independent read.

I did not verify this by comparing file hashes, and it is worth saying why. src/columnar_customscan.c differs from the head I reviewed at 64a3935 — which looks alarming for a change described as bookkeeping only. It is my own #1196 that landed in that file on main. A hash comparison across a moved base cannot tell "their contribution changed" from "the base moved underneath it", which is the lesson this branch pair taught me earlier today. So I ran it instead.

On the composed tree:

clamp comment still present                    yes
my #1196 range code present in the same file   yes (so the rebase really is the compose)
test/projection_parallel.sh   9 passed + 0 failed
test/pytest twin              9 pass + 0 fail
-- worker rows: 4534 3466  busy=2 lines=2

The worker split is 4534/3466 here, against 2276/5724 on my earlier compose and 4498/3502 on the pre-rebase base. Three runs, three splits, one verdict — both workers produce rows and busy=2, which is the property the arm asserts rather than the numbers. That is corroboration rather than a shared instrument.

Census checked with two instruments on an independently cloned tree, since the same command derived all three sibling PRs:

file says 468/403    pytest's own total 468/403    --pgc-expect-tests accepted
ledger never 1499 = declared 1499    duplicate (suite, part, check) keys: 0

The clamp note I asked for on the previous head survived the rebase intact, which was the only substantive thing I had outstanding here.

Merging remains @jdatcmd's call.

@jdatcmd
jdatcmd merged commit ee52910 into commandprompt:main Sep 22, 2026
15 checks passed
jdatcmd added a commit to linuxhikerpm/pgcolumnar that referenced this pull request Sep 22, 2026
…pt#1155)

Original work by @linuxhikerpm; rebuilt on ee52910 by @jdatcmd after five PRs
landed under it, with the review fix and the census re-derived.

THE CHANGE. rel->pages is the whole relation file, base plus every projection,
so a covering scan that reads only one projection's row groups was priced for
pages it never touches. It is now priced from that projection's own pages,
walked from the catalog, with rel->pages as the fallback when the lookup fails
so a miss can never look cheaper than the base scan.

THE REVIEW FIX. The miss arm divided a plan cost measured AFTER the fixture
clears proj_storage_id by one measured BEFORE it. Those are two catalog states,
and commandprompt#1180 made them two prices, because its sibling-pages walk reads that same
key:

    before   -- miss_run=41991.6 base_run=22000 miss_ratio=1.909   FAIL
    after    -- miss_run=41991.6 base_run=42000 miss_ratio=1.000   9/0

The comment claimed the property that failed -- "independent of how rel->pages
is computed (survives commandprompt#1180)" -- which is true of the covering arm, whose
want_run comes from the catalog, and false of this one, whose oracle is a
measured plan cost. Both halves now say which arm it is true of.

THE REORDER BOUGHT NO BLINDNESS. With pgcolumnar_projection_pages mutated to
always return fallbackPages the covering arm goes red at full strength
(`off-band got=21995.6 want=20000`); restored, 9 passed.

REBUILT RATHER THAN REPLAYED. A six-commit rebase conflicted on TESTS.md at
every step and one attempt COMMITTED FOUR CONFLICT MARKERS before being caught,
so the branch's own changes were applied to main file by file instead.
src/columnar_customscan.c needed a real merge: commandprompt#1127 renamed `scale` to
`projScale` in the same block this change rewrites, and the result keeps
commandprompt#1127's name with this change's pricing.

ALL THREE COUPLED SUITES PASS ON THE COMPOSED TREE, which is the point:

    projection_scan_io.sh    9/0   miss_ratio=1.000
    projection_parallel.sh   9/0   (commandprompt#1127, shares the function)
    base_scan_io.sh          9/0   ratio=1.000 (commandprompt#1180, whose walk reads the key)

CENSUS RE-DERIVED TWICE, once per rebase:

    cluster_tests              468 -> 469   collection, 49 cluster files
    guard_tests                403          main's, untouched
    checks_never_observed_red 1499 -> 1506   awk over the ledger
    suites_not_covered         249          unchanged

check_ledger.tsv checked rather than trusted: 1535 rows, zero duplicate
(suite, part, check) keys. TESTS.md rebuilt from main with the section applied
once and its number derived as max + 1 -- 78, having been 74 and then 77 as
main moved twice under it. 78 sections, 78 TOC entries, 78 of 78 pairing on
number AND title, contiguous, zero bad anchors.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
jdatcmd added a commit to linuxhikerpm/pgcolumnar that referenced this pull request Sep 22, 2026
…commandprompt#1155)

commandprompt#1127's comment says the ioRunProj clamp is unreachable "with projRun =
serialRun * projScale", and names "computed independently (for example from the
projection's own pages)" as what would make it live. That is this PR's title,
and my rebuild kept the comment while replacing the formula it quotes. Caught
by @OffgridwithJD.

The premise is now stated as FALSE, and reachability as UNPROVEN, because that
is what was measured rather than argued. They probed the clamp: reached three
times in projection_parallel.sh and bound zero, margins 24.4794 against
2122.2110 and 0.2473 against 163.8619; a fixture built to bind it reached once
and still did not, 0.2504 against 148.2537. Binding needs

    2*ioBase - serialRun > baseSurvival * seq_page_cost * projPages

in which sel cancels, and both attempts moved the margin the wrong way, 87x and
then 592x, because pgcolumnar_scan_io_run_cost prices only the columns read.

The clamp stays: it is cheap and its absence would be a silently negative
cpuRunProj.

    projection_scan_io.sh   9/0
    projection_parallel.sh  9/0

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
jdatcmd added a commit to linuxhikerpm/pgcolumnar that referenced this pull request Sep 22, 2026
…pt#1155)

Original work by @linuxhikerpm; rebuilt on the current main by @jdatcmd, with
the review fix and the census re-derived. Third rebuild: every PR on this board
touches the same four bookkeeping files, so merging any one makes the rest
DIRTY.

THE CHANGE. rel->pages is the whole relation file, base plus every projection,
so a covering scan that reads only one projection's row groups was priced for
pages it never touches. It is now priced from that projection's own pages, with
rel->pages as the fallback when the lookup fails so a miss can never look
cheaper than the base scan.

THE REVIEW FIX. The miss arm divided a plan cost measured AFTER the fixture
clears proj_storage_id by one measured BEFORE it -- two catalog states, which
commandprompt#1180 made two prices because its sibling walk reads that same key:

    before   miss_run=41991.6 base_run=22000 miss_ratio=1.909   FAIL
    after    miss_run=41991.6 base_run=42000 miss_ratio=1.000   9/0

And the reorder bought no blindness: with pgcolumnar_projection_pages mutated
to always return fallbackPages the covering arm goes red at full strength.

THE COMMENT THIS CHANGE FALSIFIES IS CORRECTED RATHER THAN CARRIED. commandprompt#1127 wrote
that the ioRunProj clamp is unreachable "with projRun = serialRun * projScale"
and named "computed independently (for example from the projection's own
pages)" as what would make it live. That is this change. It now records three
states: the old premise is FALSE, reachability is UNPROVEN (@OffgridwithJD
probed it, reached 3 and bound 0, then built a fixture that reached once and
still did not bind), and the clamp stays because its absence allows a silently
negative cpuRunProj -- a reason that survives whichever way reachability goes.

cluster_tests re-derived on the composed tree: 476. It has read 464, 468, 469
and now 476 as main went 463, 467, 468, 475 under this branch. Every one was
correct for the main of its hour.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
jdatcmd pushed a commit that referenced this pull request Sep 23, 2026
…1209)

#1127 called the clamp unreachable "with projRun = serialRun * projScale".
#1155 computed projRun independently, which is the falsifier #1127 named,
and the comment has read UNPROVEN since. Measured, with a probe at the
clamp site.

IT IS REACHABLE. Both sides are linear in seq_page_cost because the CPU
term is not, so three constants fitted from six points predict the crossing:

    pre = ioRun * projScale = spc * A          A = 3.1
    projRun                 = C + spc * B      B = 3.0   C = 262.5
    binding needs spc > C/(A-B) = 2625

Predicted before it was run. 2048 does not bind, missing by 0.9%; 4096
does, and the plan changes from Gather -> Parallel Custom Scan to a serial
Custom Scan, which is the consequence #1127 wrote down.

AT THE DEFAULT GUCS IT CANNOT BIND, for a constant rather than a property
of the fixture: binding needs the projection to save more than 5.12*W + 82
bytes per row, because cpu_operator_cost * W/4 is 5.12 times
seq_page_cost * W/8192. Measured storage runs 0.13x and 0.04x of W.

A second fixture built to move that margin did not move it: 2 pages of
difference and the same 2625 threshold in both, because the base compresses
the same data almost as well as the projection does.

THE CLAMP CHANGES NO PLAN. Removing it leaves every plan in
projection_parallel.sh identical and the suite green -- the unclamped total
is LARGER, so the serial covering path wins either way. It keeps cpuRunProj
from going negative, which no plan exposes.

So the two new arms are named for I/O amortisation rather than for the
clamp. I wrote them as clamp arms first; the removal mutation did not
redden them, which would have shipped a vacuous guard. What does redden the
second arm is (ioRunProj + cpuRunProj) / divisor:

    got [gather+projection] want [projection-only]

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants